Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: only initialize openssl once #29999

Closed
wants to merge 1 commit into from
Closed

src: only initialize openssl once #29999

wants to merge 1 commit into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Oct 16, 2019 •
edited
Loading

For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

Fixes: #29702

See:

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Oct 16, 2019
@sam-github sam-github added the tls Issues and PRs related to the tls subsystem. label Oct 16, 2019
@richardlau
Copy link
Member

For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

I've added a dont-land-on-v10.x label based on the above. Feel free to remove if incorrect.

@richardlau
Copy link
Member

Is there anyone we should ping from Electron in case this affects their use of BoringSSL?

@sam-github
Copy link
Contributor Author

@danbev @codebytere This affects OpenSSL initialization, you might want to take a look.

richardlau
richardlau reviewed Oct 16, 2019
View reviewed changes
src/node_crypto.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26072/

@codebytere
Copy link
Member

OPENSSL_INIT_set_config_filename is incompatible with BoringSSL, as is OPENSSL_INIT_free it seems :( Is there a potential alternative that doesn't use methods not exposed by BoringSSL?

cc @davidben for potential ideas :)

@davidben
Copy link
Contributor

Stub versions of those functions in BoringSSL would be just fine. We only add compatibility functions as we need them and this is the first time I've ever seen anyone use OPENSSL_INIT_SETTINGS.

If it's off in a corner, clearly a no-op, and doesn't affect anything else, we generally just freely add compatibility functions.

@davidben
Copy link
Contributor

We would, of course, ignore whatever config file you pass in, but that's totally self-consistent. BoringSSL has zero config file options so we vacuously "parse" the file. :-)

@sam-github
Copy link
Contributor Author

I don't mind 1b8d782d58 if its going to be a while before BoringSSL implements the compat APIs.

@codebytere What's your preference?

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26082/

@sam-github sam-github force-pushed the modernize-openssl-init branch from 1b8d782 to 21859d4 Compare October 17, 2019 18:08
@sam-github
src: only initialize openssl once
12cbdcb 
For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

Fixes: nodejs#29702

See:
- https://mta.openssl.org/pipermail/openssl-users/2019-October/011303.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_ssl.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_crypto.html
@sam-github sam-github force-pushed the modernize-openssl-init branch from 21859d4 to 12cbdcb Compare October 17, 2019 18:09
@richardlau richardlau mentioned this pull request Oct 17, 2019
Closed
2 tasks
@codebytere
Copy link
Member

@sam-github i can handle this on the BoringSSL side, so all clear 🚀 ty for tagging me in!

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26084/

@codebytere
Copy link
Member

Failing test es-module/test-esm-specifiers looks unrelated?

@sam-github
Copy link
Contributor Author

@nodejs/modules-active-members Any comment on above? Is that test flaky? It doesn't have any crypto dependency I can think of, am I missing something?

@sam-github
Copy link
Contributor Author

FTR:

=== release test-esm-specifiers ===
272Path: es-module/test-esm-specifiers
273--- stderr ---
274(node:31666) ExperimentalWarning: The ESM module loader is experimental.
275internal/modules/cjs/loader.js:1039
276      internalBinding('errors').triggerUncaughtException(
277                                ^
278
279AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
280
281'esm' !== 'cjs'
282
283    at file:///home/travis/build/nodejs/node/test/es-module/test-esm-specifiers.mjs:17:8
284    at ModuleJob.run (internal/modules/esm/module_job.js:109:37)
285    at async Loader.import (internal/modules/esm/loader.js:132:24) {
286  generatedMessage: true,
287  code: 'ERR_ASSERTION',
288  actual: 'esm',
289  expected: 'cjs',
290  operator: 'strictEqual'
291}
292Command: out/Release/node --experimental-modules --es-module-specifier-resolution=node /home/travis/build/nodejs/node/test/es-module/test-esm-specifiers.mjs

@richardlau
Copy link
Member

richardlau commented Oct 17, 2019 •
edited
Loading

Failing test es-module/test-esm-specifiers looks unrelated?

It's probably some interaction between ccache and #29974 landing on master (Travis runs on refs/pull/29999/merge created by GitHub). I'll clear the cache for this PR and restart the Travis job.

Edit: and everything now passed 🎉.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26088/

@sam-github
Copy link
Contributor Author

Resume failed on test.parallel/test-worker-prof in windows, @nodejs/platform-windows @nodejs/workers

Trying again.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26102/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26107/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26108/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26110/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 19, 2019 •
edited by Trott
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/26111/ ✔️

@Trott
Copy link
Member

Trott commented Oct 19, 2019

Landed in 8425183

@Trott Trott closed this Oct 19, 2019
Trott pushed a commit that referenced this pull request Oct 19, 2019
@sam-github @Trott
src: initialize openssl only once
8425183 
For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

Fixes: #29702

See:
- https://mta.openssl.org/pipermail/openssl-users/2019-October/011303.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_ssl.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_crypto.html

PR-URL: #29999
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
@sam-github @MylesBorins
src: initialize openssl only once
d16d673 
For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

Fixes: #29702

See:
- https://mta.openssl.org/pipermail/openssl-users/2019-October/011303.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_ssl.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_crypto.html

PR-URL: #29999
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
@sam-github @MylesBorins
src: initialize openssl only once
b019ccd 
For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

Fixes: #29702

See:
- https://mta.openssl.org/pipermail/openssl-users/2019-October/011303.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_ssl.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_crypto.html

PR-URL: #29999
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 23, 2019
Merged
targos pushed a commit that referenced this pull request Nov 8, 2019
@sam-github @targos
src: initialize openssl only once
8d8ef16 
For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

Fixes: #29702

See:
- https://mta.openssl.org/pipermail/openssl-users/2019-October/011303.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_ssl.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_crypto.html

PR-URL: #29999
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
targos pushed a commit that referenced this pull request Nov 10, 2019
@sam-github @targos
src: initialize openssl only once
70c46d1 
For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

Fixes: #29702

See:
- https://mta.openssl.org/pipermail/openssl-users/2019-October/011303.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_ssl.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_crypto.html

PR-URL: #29999
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
@targos targos mentioned this pull request Nov 10, 2019
Merged
targos pushed a commit that referenced this pull request Nov 11, 2019
@sam-github @targos
src: initialize openssl only once
aa0aacb 
For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

Fixes: #29702

See:
- https://mta.openssl.org/pipermail/openssl-users/2019-October/011303.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_ssl.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_crypto.html

PR-URL: #29999
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
@sam-github sam-github deleted the modernize-openssl-init branch November 28, 2019 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau left review comments

@codebytere codebytere codebytere approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Loading OpenSSL config files twice
7 participants
@sam-github @richardlau @nodejs-github-bot @codebytere @davidben @Trott @jasnell